Skip to content

Conversation

@matthewdavidrodgers
Copy link
Contributor

@matthewdavidrodgers matthewdavidrodgers commented May 23, 2025

WC-3584

The Router worker now limits requests when free users are over their daily limit. This decision is made via the new optional param binding (EYEBALL_ROUTER_CONFIG) that is passed in. The Router worker then denies all requests to the user worker when that param indicates limiting should occur.


  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because:
  • Wrangler / Vite E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: handled by others
  • Wrangler V3 Backport
    • TODO (before merge)
    • Wrangler PR:
    • Not necessary because: not a Wrangler change

@matthewdavidrodgers matthewdavidrodgers requested review from a team as code owners May 23, 2025 22:46
@changeset-bot
Copy link

changeset-bot bot commented May 23, 2025

🦋 Changeset detected

Latest commit: a1cd3f8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@cloudflare/workers-shared Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@matthewdavidrodgers matthewdavidrodgers force-pushed the matthewrodgers/WC-3584-free-tier-limiting branch from 51da078 to 28f55f4 Compare May 30, 2025 21:44
@matthewdavidrodgers
Copy link
Contributor Author

This is rebased over #9416 and thus the diff includes those commits as well until it is merged

@matthewdavidrodgers matthewdavidrodgers force-pushed the matthewrodgers/WC-3584-free-tier-limiting branch 2 times, most recently from e3cc92b to 4f4956f Compare June 2, 2025 21:36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Product approved this messaging, yeah? Looks good to me — just checking.

Do we want to snazz it up a bit with some CSS/illustration at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i expected this to have plenty of product feedback here and to iterate based on that. i did no styling (or even markup really) and just brought over the copy of what's currently in FL. happy to do a basic pass first


const maybeSecondRequest = request.clone();

let asset: "static_routing" | "ignored" | true | false = false;
Copy link
Contributor

@GregBrimble GregBrimble Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love mutable and non-linear variables like this (though absolutely have used them in a few places myself!)

Is it at all cleaner if this is becomes an arg to the routeToUserWorker and routeToAssets functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleaner is relative here IMO. the typing becomes cleaner but the call sites is more verbose. no strong opinion here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan of this variable being mutated and used in different places. Can we make this a parameter instead?

diff --git a/packages/workers-shared/router-worker/src/worker.ts b/packages/workers-shared/router-worker/src/worker.ts
index d88375ffe..9c188c36b 100644
--- a/packages/workers-shared/router-worker/src/worker.ts
+++ b/packages/workers-shared/router-worker/src/worker.ts
@@ -104,9 +104,7 @@ export default {
 
 			const maybeSecondRequest = request.clone();
 
-			let asset: "static_routing" | "ignored" | true | false = false;
-
-			const routeToUserWorker = async () => {
+			const routeToUserWorker = async ({ asset }: { asset: AssetTag }) => {
 				if (!config.has_user_worker) {
 					throw new Error(
 						"Fetch for user worker without having a user worker binding"
@@ -126,19 +124,19 @@ export default {
 				return env.JAEGER.enterSpan("dispatch_worker", async (span) => {
 					span.setTags({
 						hasUserWorker: true,
-						asset: asset,
+						asset,
 						dispatchType: DISPATCH_TYPE.WORKER,
 					});
 					return env.USER_WORKER.fetch(maybeSecondRequest);
 				});
 			};
 
-			const routeToAssets = async () => {
+			const routeToAssets = async ({ asset }: { asset: AssetTag }) => {
 				analytics.setData({ dispatchtype: DISPATCH_TYPE.ASSETS });
 				return await env.JAEGER.enterSpan("dispatch_assets", async (span) => {
 					span.setTags({
 						hasUserWorker: config.has_user_worker,
-						asset: asset,
+						asset,
 						dispatchType: DISPATCH_TYPE.ASSETS,
 					});
 
@@ -160,8 +158,7 @@ export default {
 					analytics.setData({
 						staticRoutingDecision: STATIC_ROUTING_DECISION.ROUTED,
 					});
-					asset = "static_routing";
-					return await routeToAssets();
+					return await routeToAssets({ asset: "static_routing" });
 				}
 				// evaluate "include" rules
 				const includeRulesMatcher = generateStaticRoutingRuleMatcher(
@@ -181,8 +178,7 @@ export default {
 					analytics.setData({
 						staticRoutingDecision: STATIC_ROUTING_DECISION.ROUTED,
 					});
-					asset = "static_routing";
-					return await routeToUserWorker();
+					return await routeToUserWorker({ asset: "static_routing" });
 				}
 
 				analytics.setData({
@@ -195,19 +191,17 @@ export default {
 			// User's configuration indicates they want user-Worker to run ahead of any
 			// assets. Do not provide any fallback logic.
 			if (config.invoke_user_worker_ahead_of_assets) {
-				asset = "ignored";
-				return await routeToUserWorker();
+				return await routeToUserWorker({ asset: "ignored" });
 			}
 
 			// If we have a user-Worker, but no assets, dispatch to Worker script
 			const assetsExist = await env.ASSET_WORKER.unstable_canFetch(request);
-			asset = assetsExist;
 			if (config.has_user_worker && !assetsExist) {
-				return await routeToUserWorker();
+				return await routeToUserWorker({ asset: assetsExist });
 			}
 
 			// Otherwise, we either don't have a user worker, OR we have matching assets and should fetch from the assets binding
-			return await routeToAssets();
+			return await routeToAssets({ asset: assetsExist });
 		} catch (err) {
 			if (userWorkerInvocation) {
 				// Don't send user Worker errors to sentry; we have no way to distinguish between
@@ -228,3 +222,5 @@ export default {
 		}
 	},
 };
+
+type AssetTag = "static_routing" | "ignored" | true | false;

Copy link
Contributor

@GregBrimble GregBrimble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the fail-closed coming hard on even for existing projects is a blocker to this. But the rest of the code (last 4 commits) looks totally fine to me.

@matthewdavidrodgers matthewdavidrodgers force-pushed the matthewrodgers/WC-3584-free-tier-limiting branch from 4f4956f to 88f8f6d Compare June 3, 2025 01:21
@matthewdavidrodgers matthewdavidrodgers force-pushed the matthewrodgers/WC-3584-free-tier-limiting branch from 88f8f6d to 3883a66 Compare June 11, 2025 19:34
@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Jun 11, 2025
@matthewdavidrodgers matthewdavidrodgers force-pushed the matthewrodgers/WC-3584-free-tier-limiting branch from 3883a66 to 36e8672 Compare June 13, 2025 00:21
@matthewdavidrodgers matthewdavidrodgers added the e2e Run wrangler + vite-plugin e2e tests on a PR label Jun 13, 2025
@pkg-pr-new
Copy link

pkg-pr-new bot commented Jun 13, 2025

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@9360

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@9360

miniflare

npm i https://pkg.pr.new/miniflare@9360

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@9360

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@9360

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@9360

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@9360

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@9360

wrangler

npm i https://pkg.pr.new/wrangler@9360

commit: a1cd3f8

@matthewdavidrodgers matthewdavidrodgers force-pushed the matthewrodgers/WC-3584-free-tier-limiting branch 2 times, most recently from 23aec17 to 3dfe021 Compare June 16, 2025 21:57
@petebacondarwin petebacondarwin self-assigned this Jun 17, 2025

const maybeSecondRequest = request.clone();

let asset: "static_routing" | "ignored" | true | false = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan of this variable being mutated and used in different places. Can we make this a parameter instead?

diff --git a/packages/workers-shared/router-worker/src/worker.ts b/packages/workers-shared/router-worker/src/worker.ts
index d88375ffe..9c188c36b 100644
--- a/packages/workers-shared/router-worker/src/worker.ts
+++ b/packages/workers-shared/router-worker/src/worker.ts
@@ -104,9 +104,7 @@ export default {
 
 			const maybeSecondRequest = request.clone();
 
-			let asset: "static_routing" | "ignored" | true | false = false;
-
-			const routeToUserWorker = async () => {
+			const routeToUserWorker = async ({ asset }: { asset: AssetTag }) => {
 				if (!config.has_user_worker) {
 					throw new Error(
 						"Fetch for user worker without having a user worker binding"
@@ -126,19 +124,19 @@ export default {
 				return env.JAEGER.enterSpan("dispatch_worker", async (span) => {
 					span.setTags({
 						hasUserWorker: true,
-						asset: asset,
+						asset,
 						dispatchType: DISPATCH_TYPE.WORKER,
 					});
 					return env.USER_WORKER.fetch(maybeSecondRequest);
 				});
 			};
 
-			const routeToAssets = async () => {
+			const routeToAssets = async ({ asset }: { asset: AssetTag }) => {
 				analytics.setData({ dispatchtype: DISPATCH_TYPE.ASSETS });
 				return await env.JAEGER.enterSpan("dispatch_assets", async (span) => {
 					span.setTags({
 						hasUserWorker: config.has_user_worker,
-						asset: asset,
+						asset,
 						dispatchType: DISPATCH_TYPE.ASSETS,
 					});
 
@@ -160,8 +158,7 @@ export default {
 					analytics.setData({
 						staticRoutingDecision: STATIC_ROUTING_DECISION.ROUTED,
 					});
-					asset = "static_routing";
-					return await routeToAssets();
+					return await routeToAssets({ asset: "static_routing" });
 				}
 				// evaluate "include" rules
 				const includeRulesMatcher = generateStaticRoutingRuleMatcher(
@@ -181,8 +178,7 @@ export default {
 					analytics.setData({
 						staticRoutingDecision: STATIC_ROUTING_DECISION.ROUTED,
 					});
-					asset = "static_routing";
-					return await routeToUserWorker();
+					return await routeToUserWorker({ asset: "static_routing" });
 				}
 
 				analytics.setData({
@@ -195,19 +191,17 @@ export default {
 			// User's configuration indicates they want user-Worker to run ahead of any
 			// assets. Do not provide any fallback logic.
 			if (config.invoke_user_worker_ahead_of_assets) {
-				asset = "ignored";
-				return await routeToUserWorker();
+				return await routeToUserWorker({ asset: "ignored" });
 			}
 
 			// If we have a user-Worker, but no assets, dispatch to Worker script
 			const assetsExist = await env.ASSET_WORKER.unstable_canFetch(request);
-			asset = assetsExist;
 			if (config.has_user_worker && !assetsExist) {
-				return await routeToUserWorker();
+				return await routeToUserWorker({ asset: assetsExist });
 			}
 
 			// Otherwise, we either don't have a user worker, OR we have matching assets and should fetch from the assets binding
-			return await routeToAssets();
+			return await routeToAssets({ asset: assetsExist });
 		} catch (err) {
 			if (userWorkerInvocation) {
 				// Don't send user Worker errors to sentry; we have no way to distinguish between
@@ -228,3 +222,5 @@ export default {
 		}
 	},
 };
+
+type AssetTag = "static_routing" | "ignored" | true | false;

Comment on lines +23 to +28
export const EyeballRouterConfigSchema = z.union([
z.object({
limitedAssetsOnly: z.boolean().optional(),
}),
z.null(),
]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why support both undefined and null. I don't believe there is any semantic difference here? Can we just drop the null and simplify this and the RequiredEyeballRouterConfig type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the optional param binding which provides this param (as the EYEBALL_CONFIG binding) provides the json when present, otherwise the value is presented in the environment as null, so that union reflects that implementation

the field limitedAssetsOnly within the object is optional to be defensive, but currently the code providing the object always sets the field when providing the object. this can be changed, but i prefer being defensive, as this config may be extended and not all fields may be set in the future

@matthewdavidrodgers
Copy link
Contributor Author

@petebacondarwin yes, this will be observed when there is a run_worker_first value that dictates running the worker. however, this change also will affect requests that hit the user worker without a run_worker_first value, particularly when limited and falling back to the user worker when an asset is not found. e.g.
GET /missing-asset.html -> asset worker reports not found, fall back to the user worker, fail-open, 404
vs
GET /missing-asset.html -> asset worker reports not found, fall back to the user worker, fail-closed, 429

This is still the right choice in my opinion, as implicitly disabling your compute when you trip a limit is unexpected and certainly a footgun (even if that compute is usually just returning a 404 for the fallback). And of course, setting not_found_handling to single-page-application or 404-page means we won't even hit those fallbacks, no limited behavior will be encountered. and yes, this all only applies to free tier customers.

@matthewdavidrodgers matthewdavidrodgers force-pushed the matthewrodgers/WC-3584-free-tier-limiting branch 2 times, most recently from 01fa580 to 23712be Compare June 17, 2025 17:42
} from "../../utils/types";
import type { ColoMetadata, Environment, ReadyAnalytics } from "./types";

const limitedResponse = `<html>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

styling and wording approved now? It's a bit of a departure from our usual styling.

Copy link
Contributor

@GregBrimble GregBrimble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style! 💅

Centralizing some of these pieces will make it easier to add free tier
limiting
Provided via a new optional param binding. When provided, it contains
info about free tier limiting for this account.
Returns an html page (included as direct module, via esbuild) with a 429
status code, mirroring the current free tier limiting behavior. It also
sets a new field in our analytics so we can observe free tier invocation
denials.

This page can evolve as we need it to, this is just my first pass at the
response.
There's no loader configured for html files by default in wrangler it
seems. So rather than mess with loaders or a custom build, inlining this
page is easy enough
@emily-shen emily-shen force-pushed the matthewrodgers/WC-3584-free-tier-limiting branch from 23712be to a1cd3f8 Compare June 18, 2025 12:41
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is already in production and working. Let's land this and then we can discuss any tidy-up refactorings that I would like to see afterwards. Wrong PR sorry.

@matthewdavidrodgers
Copy link
Contributor Author

this PR has been superseded by cloudflare/cloudflare-docs#23008

@emily-shen emily-shen closed this Jun 20, 2025
@github-project-automation github-project-automation bot moved this from Approved to Done in workers-sdk Jun 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

e2e Run wrangler + vite-plugin e2e tests on a PR

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants